profile: add profile_finish_ts #17636

sluongng · 2023-03-01T10:31:18Z

"date" field in Bazel json profile is a confusing field.

It uses Java Date.toString() output which includes local timezone
that could be hard to parse.
The name is misleading since users could mistook it for profile
start time, while it actually means the finish/end time, when
Bazel calls the writer to serialize profiling data to disk.

Add "profile_finish_ts" which has a clearer name and more consistent
value with the time unit being used in the rest of the JSON
profile(microseconds).

We shall deprecate the "date" field in a separate patch in a major
release.

sluongng · 2023-03-08T12:33:59Z

@fweikert I think you own the Bazel profiling code, could you please take a look?

sgowroji · 2023-03-08T12:44:58Z

Hi @sluongng, Its already shared with @fweikert for review. I will import once approved. Thanks !

src/main/java/com/google/devtools/build/lib/profiler/JsonTraceFileWriter.java

sgowroji · 2023-03-24T11:04:41Z

Hi @sluongng, Could you please respond to the review comments and changes. Thanks!

sluongng · 2023-03-26T15:25:21Z

It has been relatively long time so I lost part of the context for this change.

Took me a bit to review this to see why I needed it: For translating Bazel Profile to Open Telemetry data format, I need a parable time stamp.

Updated according to review feedbacks.

meisterT · 2023-04-13T10:10:47Z

site/en/configure/json-trace-profile.md

@@ -167,7 +167,7 @@ Example:
 {
 "otherData": {
 "build_id": "101bff9a-7243-4c1a-8503-9dc6ae4c3b05",
- "date": "Wed Oct 26 08:22:35 CEST 2022",


please keep the date in the example

"date" field in Bazel json profile is a confusing field. - It uses Java Date.toString() output which includes local timezone that could be hard to parse. - The name is misleading since users could mistook it for profile start time, while it actually means the finish/end time, when Bazel calls the writer to serialize profiling data to disk. Add "profile_finish_ts" which has a clearer name and more consistent value with the time unit being used in the rest of the JSON profile(microseconds). We shall deprecate the "date" field in a separate patch in a major release.

sluongng · 2023-04-16T09:10:42Z

@meisterT I updated both the diff and the commit message / PR summary accordingly.

Do you want to introduce the deprecation patch or shall I?
I do think removing the ambiguous field will provide a much better UX vs leaving it there.

meisterT · 2023-04-17T09:23:53Z

@meisterT I updated both the diff and the commit message / PR summary accordingly.

Thanks!

Do you want to introduce the deprecation patch or shall I? I do think removing the ambiguous field will provide a much better UX vs leaving it there.

Feel free to send a separate PR for this!

brentleyjones · 2023-04-17T12:47:45Z

@bazel-io flag

keertk · 2023-04-17T22:15:34Z

@bazel-io fork 6.2.0

keertk · 2023-04-17T22:37:43Z

Hi @sluongng if we want to include this in 6.2.0, it looks like some other commits need to be cherry-picked first (e.g. eb279aa?). Could you please submit a PR against the release-6.2.0 branch with the required changes? Or let me know what's needed and I can do it. Thanks!
cc @meisterT

meisterT · 2023-04-18T07:23:40Z

There were many changes we did not cherry pick into 6.1. If we want this functionality in 6.2, I think the easiest is to repeat the spirit of this change instead of cherrypicking other changes.

sluongng · 2023-04-18T08:46:13Z

Recreated in #18129

Repeating change from bazelbuild#17636 for 6.2 release

"date" field in Bazel json profile is a confusing field. - It uses Java Date.toString() output which includes local timezone that could be hard to parse. - The name is misleading since users could mistook it for profile start time, while it actually means the finish/end time, when Bazel calls the writer to serialize profiling data to disk. Add "profile_finish_ts" which has a clearer name and more consistent value with the time unit being used in the rest of the JSON profile(microseconds). We shall deprecate the "date" field in a separate patch in a major release. Closes bazelbuild#17636. PiperOrigin-RevId: 524817808 Change-Id: I89e144d42f0e608818507f5f9f9bc525c9dc7ac3

timothyg-stripe · 2024-05-22T18:56:30Z

Hi @sluongng, I don't think this change is correct. You wrote that

The name is misleading since users could mistook it for profile start time, while it actually means the finish/end time, when Bazel calls the writer to serialize profiling data to disk.

but this method is actually called at the beginning of the Bazel process.

Additionally, profile_finish_ts lacks precision compared to date. On JDK 17, Instant.now().toString() gives us microsecond precision on Linux, while profile_finish_ts is rounded to the second.

Edit: Filed a ticket: #22505

sluongng requested review from gregestren and fweikert as code owners March 1, 2023 10:31

sgowroji added team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Mar 1, 2023

sgowroji self-requested a review March 6, 2023 09:48

meisterT requested changes Mar 20, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/profiler/JsonTraceFileWriter.java Outdated Show resolved Hide resolved

meisterT reviewed Mar 20, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/profiler/JsonTraceFileWriter.java Outdated Show resolved Hide resolved

sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 21, 2023

sluongng requested a review from philomathing as a code owner March 26, 2023 15:17

sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Mar 28, 2023

sgowroji removed their request for review March 28, 2023 10:49

keertk requested a review from meisterT March 28, 2023 18:03

meisterT reviewed Apr 13, 2023

View reviewed changes

sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Apr 13, 2023

sluongng force-pushed the sluongng/profile-otherdata-adjust branch from 6397870 to cbae4a4 Compare April 16, 2023 09:08

sluongng changed the title ~~profile: replace otherData.date field with profile_finish_ts~~ profile: add profile_finish_ts Apr 16, 2023

sgowroji assigned meisterT Apr 17, 2023

sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Apr 17, 2023

meisterT approved these changes Apr 17, 2023

View reviewed changes

meisterT removed the awaiting-review PR is awaiting review from an assigned reviewer label Apr 17, 2023

meisterT added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 17, 2023

copybara-service bot closed this in dbc8423 Apr 17, 2023

Pavank1992 removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 17, 2023

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 17, 2023

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Apr 17, 2023

bazel-io mentioned this pull request Apr 17, 2023

[6.2.0] profile: add profile_finish_ts #18125

Closed

sluongng mentioned this pull request Apr 18, 2023

profile: add profile_finish_ts #18129

Merged

sluongng deleted the sluongng/profile-otherdata-adjust branch April 18, 2023 08:46

sluongng added a commit to sluongng/bazel that referenced this pull request Apr 18, 2023

profile: add profile_finish_ts

5ced37d

Repeating change from bazelbuild#17636 for 6.2 release

timothyg-stripe mentioned this pull request May 22, 2024

profile_finish_ts actually describes profile start timestamp #22505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

profile: add profile_finish_ts #17636

profile: add profile_finish_ts #17636

sluongng commented Mar 1, 2023 •

edited

Loading

sluongng commented Mar 8, 2023

sgowroji commented Mar 8, 2023

sgowroji commented Mar 24, 2023

sluongng commented Mar 26, 2023

meisterT Apr 13, 2023

sluongng commented Apr 16, 2023

meisterT commented Apr 17, 2023

brentleyjones commented Apr 17, 2023

keertk commented Apr 17, 2023

keertk commented Apr 17, 2023 •

edited

Loading

meisterT commented Apr 18, 2023

sluongng commented Apr 18, 2023

timothyg-stripe commented May 22, 2024 •

edited

Loading

profile: add profile_finish_ts #17636

profile: add profile_finish_ts #17636

Conversation

sluongng commented Mar 1, 2023 • edited Loading

sluongng commented Mar 8, 2023

sgowroji commented Mar 8, 2023

sgowroji commented Mar 24, 2023

sluongng commented Mar 26, 2023

meisterT Apr 13, 2023

Choose a reason for hiding this comment

sluongng commented Apr 16, 2023

meisterT commented Apr 17, 2023

brentleyjones commented Apr 17, 2023

keertk commented Apr 17, 2023

keertk commented Apr 17, 2023 • edited Loading

meisterT commented Apr 18, 2023

sluongng commented Apr 18, 2023

timothyg-stripe commented May 22, 2024 • edited Loading

sluongng commented Mar 1, 2023 •

edited

Loading

keertk commented Apr 17, 2023 •

edited

Loading

timothyg-stripe commented May 22, 2024 •

edited

Loading